C++: Add ECMAScript std::regex parser - #22300
Conversation
Note that we are currently still implementing what Ruby thinks a regex is. This is not correct as C++ by default uses a variant of ECMAScript regexes. We will address this in the follow-up commits.
Strings can have a prefix in C++, which affects the location.
Clean up test source while here
There was a problem hiding this comment.
Pull request overview
Adds an ECMAScript-compatible std::regex parser and parse-tree model for future C++ data-flow integration.
Changes:
- Adds parser and regex tree-view libraries.
- Supports raw-string source locations and shared regex utilities.
- Adds comprehensive parsing, value, and location tests.
Show a summary per file
| File | Description |
|---|---|
cpp/ql/lib/semmle/code/cpp/regex/internal/ParseRegExp.qll |
Implements regex parsing. |
cpp/ql/lib/semmle/code/cpp/regex/RegexTreeView.qll |
Exposes parsed regex trees. |
cpp/ql/lib/semmle/code/cpp/exprs/Literal.qll |
Models raw string literals. |
cpp/ql/lib/qlpack.yml |
Adds the regex dependency. |
cpp/ql/lib/change-notes/2026-07-23-std-regex-ecmascript-parser.md |
Documents the feature. |
cpp/ql/test/library-tests/regex/regexp.cpp |
Provides regex test inputs. |
cpp/ql/test/library-tests/regex/regexp.ql |
Tests parsed terms and values. |
cpp/ql/test/library-tests/regex/regexp.expected |
Records expected query results. |
cpp/ql/test/library-tests/regex/parse.ql |
Produces parse-tree graphs. |
cpp/ql/test/library-tests/regex/parse.expected |
Records expected parse trees. |
cpp/ql/test/library-tests/regex/locations.ql |
Tests source locations. |
cpp/ql/test/library-tests/regex/locations.expected |
Records expected locations. |
Review details
Tip
Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (3)
cpp/ql/lib/semmle/code/cpp/regex/RegexTreeView.qll:834
- The single-character claim contradicts this implementation and the new
abctest result, where oneRegExpConstantrepresents the complete three-character constant. Remove that claim to keep the public class documentation accurate.
* A constant regular expression term, that is, a regular expression
* term matching a single string. Currently, this will always be a single character.
cpp/ql/lib/semmle/code/cpp/regex/internal/ParseRegExp.qll:65
- Remove the accidentally duplicated explanation.
// check if the character that comes before the previous closing bracket
// is an opening bracket (taking `^` into account)
// check if the character that comes before the previous closing bracket
// is an opening bracket (taking `^` into account)
cpp/ql/lib/semmle/code/cpp/regex/RegexTreeView.qll:604
- Add the missing possessive apostrophe.
/** Holds if this terms name is given by the part following the escape character. */
- Files reviewed: 12/12 changed files
- Comments generated: 5
- Review effort level: Balanced
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Review part 1. Thank you for breaking this PR into logical commits, with commit comments explaining the changes - this was extremely helpful for making sense of the changes you've made from the Ruby implementation I'm already somewhat familiar with.
I've also created a small PR jketema#33 onto this PR, addressing an edge case with string locations that I investigated, and figured I might as well trivially fix and share my code changes for.
| std::basic_regex<char16_t> r_loc_uR(uR"(a\nc)"); | ||
| std::basic_regex<char32_t> r_loc_UR(UR"(a\nc)"); | ||
| std::regex r_loc_Rx(R"x(a\nc)x"); | ||
| std::regex r_loc_Rfoo(R"foo(a\nc)foo"); |
There was a problem hiding this comment.
Should we have test cases for invalid regexs to ensure nothing too strange happens there - and for that matter string literals that are not remotely regexs, to ensure they are not misidentified as such.
There was a problem hiding this comment.
Is there anything specific you're thinking about?
There was a problem hiding this comment.
Nothing in particular, just (1) sometimes people write invalid regexs, and sometimes libraries even accept them so they don't get spotted; (2) real world databases will contain a lot of string literals that aren't regexs, so it's important we don't do anything silly (or computationally expensive for that matter) with them. I can't remember how this worked in Ruby / Python / Swift.
There was a problem hiding this comment.
I can't remember how this worked in Ruby / Python / Swift.
I think the idea is that we only look at strings that actually flow into the correct std::regex function arguments. That is clearly missing at this point.
There was a problem hiding this comment.
I left this as-is under the assumption I stated above: I'm assuming that correct regexes need to be parsed correctly, but that for incorrect ones it doesn't really matter, as those should generally not occur because they lead to runtime exceptions in actual code.
(1) sometimes people write invalid regexs, and sometimes libraries even accept them so they don't get spotted;
This is fair. I think that if we find cases where the library accepts them, then we definitely need to update the parser to handle them correctly.
(2) real world databases will contain a lot of string literals that aren't regexs
Agreed. For some for context, note that RegExp is abstract. In the tests I currently do:
class RegExpTest extends RegExp {
RegExpTest() { any() }
}
Follow-up work should extend RegExp in the library in such a way that the charpred restricts this to only strings that flow into std:regex and friends.
Commented on that here: https://github.com/jketema/codeql/pull/33/files#r3871704321 |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Review part 2. I've now reviewed all of the code and tests.
I'm very happy with what I see, you've clearly put considerably effort into getting this right ... though experience suggests we should be prepared to find bugs and missing features when we start to implement queries on top of the library, as doing so will test it more deeply.
I'm going to do a few quick tests locally, but I'm leaning towards getting this merged quickly so we can build on it.
👍
I've now done my local testing. I noticed locations of regex terms where the regex includes one or more escape sequences ( Nothing else to review, I'm happy to approve this when everything above has been answered. I don't think anything I've asked for is critical at this stage (as no queries use the library yet), so feel free to write up a TODO list issue instead of fixing every detail now. It will be good to have some code in |
I saw that. I tried fixing that, but that turned out to be non-trivial. I'll add a note in the |
These already occur in `isIdentityEscape`.
Added a comment in 40a8746. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
I've cherry-picked your commits and resolved expected test result conflicts in the process. They occur here as 9e52f28 and 8e169d3. |
|
State as of this writing: still need to have a look at the following from jketema#33, otherwise all comments have been addressed.
Expanded on the fragility of the locations here: 50ed141 |
Commit-by-commit review recommended. This is not hooked into anything yet, as this is getting quite big already, so I didn't want to add the flow config here, or other regex alternatives that can be used with
std::regex.Approach: